From 7cf7c4611f7ac09913a3812c87c37e8d0162fe54 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Wed, 24 Jun 2020 18:07:18 +0100 Subject: [PATCH 1/4] Add tests for stacktrace file paths --- spec/stacktrace_spec.rb | 212 ++++++++++++++++++++++++++-------------- 1 file changed, 139 insertions(+), 73 deletions(-) diff --git a/spec/stacktrace_spec.rb b/spec/stacktrace_spec.rb index f17b2dff3..38339d189 100644 --- a/spec/stacktrace_spec.rb +++ b/spec/stacktrace_spec.rb @@ -1,91 +1,157 @@ require 'spec_helper' describe Bugsnag::Stacktrace do - it "includes code in the stack trace" do - begin - _a = 1 - _b = 2 - _c = 3 - "Test".prepnd "T" - _d = 4 - _e = 5 - _f = 6 - rescue Exception => e - Bugsnag.notify(e) + context "sending code" do + it "includes code in the stack trace" do + begin + _a = 1 + _b = 2 + _c = 3 + "Test".prepnd "T" + _d = 4 + _e = 5 + _f = 6 + rescue Exception => e + Bugsnag.notify(e) + end + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + exception = get_exception_from_payload(payload) + starting_line = __LINE__ - 13 + expect(exception["stacktrace"][0]["code"]).to eq({ + (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"', + (starting_line + 4).to_s => ' _d = 4', + (starting_line + 5).to_s => ' _e = 5', + (starting_line + 6).to_s => ' _f = 6' + }) + } end - expect(Bugsnag).to have_sent_notification{ |payload, headers| - exception = get_exception_from_payload(payload) - starting_line = __LINE__ - 13 - expect(exception["stacktrace"][0]["code"]).to eq({ - (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"', - (starting_line + 4).to_s => ' _d = 4', - (starting_line + 5).to_s => ' _e = 5', - (starting_line + 6).to_s => ' _f = 6' + it "allows you to disable sending code" do + Bugsnag.configuration.send_code = false + + notify_test_exception + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + exception = get_exception_from_payload(payload) + expect(exception["stacktrace"][1]["code"]).to eq(nil) + } + end + + it 'should send the first 7 lines of the file for exceptions near the top' do + load 'spec/fixtures/crashes/start_of_file.rb' rescue Bugsnag.notify $! + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + exception = get_exception_from_payload(payload) + + expect(exception["stacktrace"][0]["code"]).to eq({ + "1" => "#", + "2" => "raise 'hell'", + "3" => "#", + "4" => "#", + "5" => "#", + "6" => "#", + "7" => "#" }) - } - end + } + end - it "allows you to disable sending code" do - Bugsnag.configuration.send_code = false + it 'should send the last 7 lines of the file for exceptions near the bottom' do + load 'spec/fixtures/crashes/end_of_file.rb' rescue Bugsnag.notify $! - notify_test_exception + expect(Bugsnag).to have_sent_notification{ |payload, headers| + exception = get_exception_from_payload(payload) - expect(Bugsnag).to have_sent_notification{ |payload, headers| - exception = get_exception_from_payload(payload) - expect(exception["stacktrace"][1]["code"]).to eq(nil) - } - end + expect(exception["stacktrace"][0]["code"]).to eq({ + "3" => "#", + "4" => "#", + "5" => "#", + "6" => "#", + "7" => "#", + "8" => "raise 'hell'", + "9" => "#" + }) + } + end - it 'should send the first 7 lines of the file for exceptions near the top' do - load 'spec/fixtures/crashes/start_of_file.rb' rescue Bugsnag.notify $! - - expect(Bugsnag).to have_sent_notification{ |payload, headers| - exception = get_exception_from_payload(payload) - - expect(exception["stacktrace"][0]["code"]).to eq({ - "1" => "#", - "2" => "raise 'hell'", - "3" => "#", - "4" => "#", - "5" => "#", - "6" => "#", - "7" => "#" - }) - } - end + it 'should send the last 7 lines of the file for exceptions near the bottom' do + load 'spec/fixtures/crashes/short_file.rb' rescue Bugsnag.notify $! + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + exception = get_exception_from_payload(payload) - it 'should send the last 7 lines of the file for exceptions near the bottom' do - load 'spec/fixtures/crashes/end_of_file.rb' rescue Bugsnag.notify $! - - expect(Bugsnag).to have_sent_notification{ |payload, headers| - exception = get_exception_from_payload(payload) - - expect(exception["stacktrace"][0]["code"]).to eq({ - "3" => "#", - "4" => "#", - "5" => "#", - "6" => "#", - "7" => "#", - "8" => "raise 'hell'", - "9" => "#" - }) - } + expect(exception["stacktrace"][0]["code"]).to eq({ + "1" => "raise 'hell'" + }) + } + end end - it 'should send the last 7 lines of the file for exceptions near the bottom' do - load 'spec/fixtures/crashes/short_file.rb' rescue Bugsnag.notify $! + context "file paths" do + it "leaves absolute paths alone" do + configuration = Bugsnag::Configuration.new + configuration.send_code = false + + dir = File.dirname(__FILE__) + + backtrace = [ + "/foo/bar/app/models/user.rb:1:in `something'", + "/foo/bar/other_vendor/lib/dont.rb:2:in `to_s'", + "/foo/bar/vendor/lib/ignore_me.rb:3:in `to_s'", + "/foo/bar/.bundle/lib/ignore_me.rb:4:in `to_s'", + "#{dir}/../spec/stacktrace_spec.rb:5:in `something_else'", + ] - expect(Bugsnag).to have_sent_notification{ |payload, headers| - exception = get_exception_from_payload(payload) + stacktrace = Bugsnag::Stacktrace.new(backtrace, configuration).to_a - expect(exception["stacktrace"][0]["code"]).to eq({ - "1" => "raise 'hell'" - }) - } + expect(stacktrace).to eq([ + { file: "/foo/bar/app/models/user.rb", lineNumber: 1, method: "something" }, + { file: "/foo/bar/other_vendor/lib/dont.rb", lineNumber: 2, method: "to_s" }, + { file: "/foo/bar/vendor/lib/ignore_me.rb", lineNumber: 3, method: "to_s" }, + { file: "/foo/bar/.bundle/lib/ignore_me.rb", lineNumber: 4, method: "to_s" }, + { file: "#{dir}/../spec/stacktrace_spec.rb", lineNumber: 5, method: "something_else" }, + ]) + end + + it "does not modify relative paths if they can't be resolved" do + configuration = Bugsnag::Configuration.new + + backtrace = [ + "./foo/bar/baz.rb:1:in `something'", + "../foo.rb:1:in `to_s'", + "../xyz.rb:1:in `to_s'", + ] + + stacktrace = Bugsnag::Stacktrace.new(backtrace, configuration).to_a + + expect(stacktrace).to eq([ + { code: nil, file: "./foo/bar/baz.rb", lineNumber: 1, method: "something" }, + { code: nil, file: "../foo.rb", lineNumber: 1, method: "to_s" }, + { code: nil, file: "../xyz.rb", lineNumber: 1, method: "to_s" }, + ]) + end + + it "resolves relative paths when the files exist" do + configuration = Bugsnag::Configuration.new + configuration.send_code = false + + backtrace = [ + "./spec/spec_helper.rb:1:in `something'", + "./lib/bugsnag/breadcrumbs/../configuration.rb:100:in `to_s'", + ] + + stacktrace = Bugsnag::Stacktrace.new(backtrace, configuration).to_a + + dir = File.dirname(__FILE__) + + expect(stacktrace).to eq([ + { file: "#{dir}/spec_helper.rb", lineNumber: 1, method: "something" }, + { file: "#{File.dirname(dir)}/lib/bugsnag/configuration.rb", lineNumber: 100, method: "to_s" }, + ]) + end end context "with configurable vendor_path" do From 481adec516c896a85f6e47e2004bdd0a8a5e8492 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Wed, 24 Jun 2020 18:08:45 +0100 Subject: [PATCH 2/4] Use File.realpath over Pathname.realpath This is a significant performance boost when sending a large number of errors as it avoids the overhead of creating another Pathname object --- lib/bugsnag/stacktrace.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/bugsnag/stacktrace.rb b/lib/bugsnag/stacktrace.rb index c147935df..f4eb762ca 100644 --- a/lib/bugsnag/stacktrace.rb +++ b/lib/bugsnag/stacktrace.rb @@ -17,20 +17,19 @@ def initialize(backtrace, configuration) backtrace = caller if !backtrace || backtrace.empty? @processed_backtrace = backtrace.map do |trace| + # Parse the stacktrace line if trace.match(BACKTRACE_LINE_REGEX) file, line_str, method = [$1, $2, $3] elsif trace.match(JAVA_BACKTRACE_REGEX) method, file, line_str = [$1, $2, $3] end - # Parse the stacktrace line - next(nil) if file.nil? # Expand relative paths p = Pathname.new(file) if p.relative? - file = p.realpath.to_s rescue file + file = File.realpath(file) rescue file end # Generate the stacktrace line hash From 1c166aab2489176035bf5aad706fe90eba5ceab1 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Thu, 25 Jun 2020 09:29:28 +0100 Subject: [PATCH 3/4] Add tests for relative paths without './' or '../' --- spec/stacktrace_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/stacktrace_spec.rb b/spec/stacktrace_spec.rb index 38339d189..0f0487edd 100644 --- a/spec/stacktrace_spec.rb +++ b/spec/stacktrace_spec.rb @@ -123,6 +123,7 @@ "./foo/bar/baz.rb:1:in `something'", "../foo.rb:1:in `to_s'", "../xyz.rb:1:in `to_s'", + "abc.rb:1:in `defg'", ] stacktrace = Bugsnag::Stacktrace.new(backtrace, configuration).to_a @@ -131,6 +132,7 @@ { code: nil, file: "./foo/bar/baz.rb", lineNumber: 1, method: "something" }, { code: nil, file: "../foo.rb", lineNumber: 1, method: "to_s" }, { code: nil, file: "../xyz.rb", lineNumber: 1, method: "to_s" }, + { code: nil, file: "abc.rb", lineNumber: 1, method: "defg" }, ]) end @@ -141,6 +143,7 @@ backtrace = [ "./spec/spec_helper.rb:1:in `something'", "./lib/bugsnag/breadcrumbs/../configuration.rb:100:in `to_s'", + "lib/bugsnag.rb:20:in `notify'", ] stacktrace = Bugsnag::Stacktrace.new(backtrace, configuration).to_a @@ -150,6 +153,7 @@ expect(stacktrace).to eq([ { file: "#{dir}/spec_helper.rb", lineNumber: 1, method: "something" }, { file: "#{File.dirname(dir)}/lib/bugsnag/configuration.rb", lineNumber: 100, method: "to_s" }, + { file: "#{File.dirname(dir)}/lib/bugsnag.rb", lineNumber: 20, method: "notify" }, ]) end end From 47e67b715713f9f2bb5cca023b60a41f711d9719 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Tue, 14 Jul 2020 11:56:33 +0100 Subject: [PATCH 4/4] Add changelog entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08799dd3e..86d14717e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,9 @@ Changelog * Improve performance of payload cleaning | [#601](https://github.com/bugsnag/bugsnag-ruby/pull/601) +* Improve performance when processing stacktraces + | [#602](https://github.com/bugsnag/bugsnag-ruby/pull/602) + ### Deprecated * The `ignore_classes` configuration option has been deprecated in favour of `discard_classes`. `ignore_classes` will be removed in the next major release