-
Notifications
You must be signed in to change notification settings - Fork 68
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
Implement code hunks support #258
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,7 +102,7 @@ module Patterns | |
# @param [Exception] exception The exception, which contains a backtrace to | ||
# parse | ||
# @return [Array<Hash{Symbol=>String,Integer}>] the parsed backtrace | ||
def self.parse(exception, logger) | ||
def self.parse(config, exception) | ||
return [] if exception.backtrace.nil? || exception.backtrace.none? | ||
|
||
regexp = best_regexp_for(exception) | ||
|
@@ -111,14 +111,14 @@ def self.parse(exception, logger) | |
frame = match_frame(regexp, stackframe) | ||
|
||
unless frame | ||
logger.error( | ||
config.logger.error( | ||
"can't parse '#{stackframe}' (please file an issue so we can fix " \ | ||
"it: https://github.com/airbrake/airbrake-ruby/issues/new)" | ||
) | ||
frame = { file: nil, line: nil, function: stackframe } | ||
end | ||
|
||
stack_frame(frame) | ||
stack_frame(config, frame) | ||
end | ||
end | ||
|
||
|
@@ -176,10 +176,15 @@ def execjs_exception?(exception) | |
end | ||
# rubocop:enable Metrics/PerceivedComplexity, Metrics/CyclomaticComplexity | ||
|
||
def stack_frame(match) | ||
{ file: match[:file], | ||
def stack_frame(config, match) | ||
frame = { | ||
file: match[:file], | ||
line: (Integer(match[:line]) if match[:line]), | ||
function: match[:function] } | ||
function: match[:function] | ||
} | ||
|
||
read_code_hunk(config.logger, frame) if config.code_hunks | ||
frame | ||
end | ||
|
||
def match_frame(regexp, stackframe) | ||
|
@@ -188,6 +193,21 @@ def match_frame(regexp, stackframe) | |
|
||
Patterns::GENERIC.match(stackframe) | ||
end | ||
|
||
def read_code_hunk(logger, frame) | ||
code_hunk = Airbrake::CodeHunk.new(frame[:file], frame[:line]).to_h | ||
return unless code_hunk | ||
|
||
unless code_hunk.key?(0) | ||
frame[:code_hunk] = code_hunk | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's rename to just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
return | ||
end | ||
|
||
logger.error( | ||
"#{LOG_LABEL} error while reading code hunk `#{file}:#{line}'. " \ | ||
"Reason: #{code_hunk[0]}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, I don't understand why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Files can never have line 0. |
||
) | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
module Airbrake | ||
## | ||
# Represents a small hunk of code consisting of a base line and a couple lines | ||
# around | ||
class CodeHunk | ||
## | ||
# @return [Integer] the maximum length of a line | ||
MAX_LINE_LEN = 200 | ||
|
||
## | ||
# @return [Integer] how many lines should be read around the base line | ||
INTERVAL = 3 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
def initialize(file, line, interval = INTERVAL) | ||
@file = file | ||
@line = line | ||
|
||
@start_line = [line - interval, 1].max | ||
@end_line = line + interval | ||
|
||
@code_hash = {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. code_hash -> lines There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
end | ||
|
||
## | ||
# @return [Hash{Integer=>String}, nil] code hunk around the base line. When | ||
# an error occurrs, returns a zero key Hash | ||
def to_h | ||
return @code_hash unless @code_hash.empty? | ||
return unless File.exist?(@file) | ||
|
||
begin | ||
fetch_code | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I see is that you check
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically, checking lines on line 29 is not needed. It's just prevention of reading the same file twice (for the same results). I removed it for now. |
||
rescue StandardError => ex | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an example you asked for. |
||
{ 0 => ex } | ||
end | ||
|
||
@code_hash | ||
end | ||
|
||
private | ||
|
||
def fetch_code | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This actually reads somethings, but it is called fetch and returns nil too. |
||
File.foreach(@file).with_index(1) do |line, i| | ||
next if i < @start_line | ||
break if i > @end_line | ||
|
||
@code_hash[i] = line[0...MAX_LINE_LEN].rstrip | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,10 @@ | ||
require 'spec_helper' | ||
|
||
RSpec.describe Airbrake::Backtrace do | ||
let(:config) do | ||
Airbrake::Config.new.tap { |c| c.logger = Logger.new('/dev/null') } | ||
end | ||
|
||
describe ".parse" do | ||
context "UNIX backtrace" do | ||
let(:parsed_backtrace) do | ||
|
@@ -23,7 +27,7 @@ | |
|
||
it "returns a properly formatted array of hashes" do | ||
expect( | ||
described_class.parse(AirbrakeTestError.new, Logger.new('/dev/null')) | ||
described_class.parse(config, AirbrakeTestError.new) | ||
).to eq(parsed_backtrace) | ||
end | ||
end | ||
|
@@ -44,9 +48,7 @@ | |
end | ||
|
||
it "returns a properly formatted array of hashes" do | ||
expect( | ||
described_class.parse(ex, Logger.new('/dev/null')) | ||
).to eq(parsed_backtrace) | ||
expect(described_class.parse(config, ex)).to eq(parsed_backtrace) | ||
end | ||
end | ||
|
||
|
@@ -71,7 +73,7 @@ | |
allow(described_class).to receive(:java_exception?).and_return(true) | ||
|
||
expect( | ||
described_class.parse(JavaAirbrakeTestError.new, Logger.new('/dev/null')) | ||
described_class.parse(config, JavaAirbrakeTestError.new) | ||
).to eq(backtrace_array) | ||
end | ||
end | ||
|
@@ -99,10 +101,7 @@ | |
|
||
it "returns a properly formatted array of hashes" do | ||
allow(described_class).to receive(:java_exception?).and_return(true) | ||
|
||
expect( | ||
described_class.parse(ex, Logger.new('/dev/null')) | ||
).to eq(parsed_backtrace) | ||
expect(described_class.parse(config, ex)).to eq(parsed_backtrace) | ||
end | ||
end | ||
|
||
|
@@ -126,9 +125,7 @@ | |
let(:ex) { AirbrakeTestError.new.tap { |e| e.set_backtrace(backtrace) } } | ||
|
||
it "returns a properly formatted array of hashes" do | ||
expect( | ||
described_class.parse(ex, Logger.new('/dev/null')) | ||
).to eq(parsed_backtrace) | ||
expect(described_class.parse(config, ex)).to eq(parsed_backtrace) | ||
end | ||
end | ||
|
||
|
@@ -153,9 +150,7 @@ | |
end | ||
|
||
it "returns a properly formatted array of hashes" do | ||
expect( | ||
described_class.parse(ex, Logger.new('/dev/null')) | ||
).to eq(parsed_backtrace) | ||
expect(described_class.parse(config, ex)).to eq(parsed_backtrace) | ||
end | ||
end | ||
|
||
|
@@ -173,9 +168,7 @@ | |
end | ||
|
||
it "returns a properly formatted array of hashes" do | ||
expect( | ||
described_class.parse(ex, Logger.new('/dev/null')) | ||
).to eq(parsed_backtrace) | ||
expect(described_class.parse(config, ex)).to eq(parsed_backtrace) | ||
end | ||
end | ||
end | ||
|
@@ -187,15 +180,15 @@ | |
|
||
it "returns array of hashes where each unknown frame is marked as 'function'" do | ||
expect( | ||
described_class.parse(ex, Logger.new('/dev/null')) | ||
described_class.parse(config, ex) | ||
).to eq([file: nil, line: nil, function: 'a b c 1 23 321 .rb']) | ||
end | ||
|
||
it "logs unknown frames as errors" do | ||
out = StringIO.new | ||
logger = Logger.new(out) | ||
config.logger = Logger.new(out) | ||
|
||
expect { described_class.parse(ex, logger) }. | ||
expect { described_class.parse(config, ex) }. | ||
to change { out.string }. | ||
from(''). | ||
to(/ERROR -- : can't parse 'a b c 1 23 321 .rb'/) | ||
|
@@ -216,9 +209,7 @@ | |
end | ||
|
||
it "returns a properly formatted array of hashes" do | ||
expect( | ||
described_class.parse(ex, Logger.new('/dev/null')) | ||
).to eq(parsed_backtrace) | ||
expect(described_class.parse(config, ex)).to eq(parsed_backtrace) | ||
end | ||
end | ||
|
||
|
@@ -241,9 +232,7 @@ | |
|
||
it "returns a properly formatted array of hashes" do | ||
stub_const('OCIError', AirbrakeTestError) | ||
expect( | ||
described_class.parse(ex, Logger.new('/dev/null')) | ||
).to eq(parsed_backtrace) | ||
expect(described_class.parse(config, ex)).to eq(parsed_backtrace) | ||
end | ||
end | ||
|
||
|
@@ -279,9 +268,7 @@ | |
stub_const('ExecJS::RuntimeError', AirbrakeTestError) | ||
stub_const('Airbrake::RUBY_20', false) | ||
|
||
expect( | ||
described_class.parse(ex, Logger.new('/dev/null')) | ||
).to eq(parsed_backtrace) | ||
expect(described_class.parse(config, ex)).to eq(parsed_backtrace) | ||
end | ||
end | ||
|
||
|
@@ -296,12 +283,46 @@ | |
stub_const('ExecJS::RuntimeError', NameError) | ||
stub_const('Airbrake::RUBY_20', true) | ||
|
||
expect( | ||
described_class.parse(ex, Logger.new('/dev/null')) | ||
).to eq(parsed_backtrace) | ||
expect(described_class.parse(config, ex)).to eq(parsed_backtrace) | ||
end | ||
end | ||
end | ||
end | ||
|
||
context "when code hunks are enabled" do | ||
let(:config) do | ||
config = Airbrake::Config.new | ||
config.logger = Logger.new('/dev/null') | ||
config.code_hunks = true | ||
config | ||
end | ||
|
||
let(:parsed_backtrace) do | ||
[ | ||
{ | ||
file: File.join(fixture_path('code.rb')), | ||
line: 94, | ||
function: 'to_json', | ||
code_hunk: { | ||
91 => ' def to_json', | ||
92 => ' loop do', | ||
93 => ' begin', | ||
94 => ' json = @payload.to_json', | ||
95 => ' rescue *JSON_EXCEPTIONS => ex', | ||
# rubocop:disable Metrics/LineLength,Lint/InterpolationCheck | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lint/UnneededDisable: Unnecessary disabling of Lint/InterpolationCheck (did you mean Lint/LiteralInCondition?). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lint/UnneededDisable: Unnecessary disabling of Lint/InterpolationCheck (did you mean Lint/LiteralInCondition?), Metrics/LineLength. |
||
96 => ' @config.logger.debug("#{LOG_LABEL} `notice.to_json` failed: #{ex.class}: #{ex}")', | ||
# rubocop:enable Metrics/LineLength,Lint/InterpolationCheck | ||
97 => ' else' | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unexpected token tRCURLY |
||
] | ||
end | ||
|
||
it "attaches code to each frame" do | ||
ex = RuntimeError.new | ||
ex.set_backtrace([File.join(fixture_path('code.rb') + ":94:in `to_json'")]) | ||
expect(described_class.parse(config, ex)).to eq(parsed_backtrace) | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see 2
unless
that check forcode_hunk
. I also don't understand whatcode_hunk.key?(0)
does so it would be nice if you refactor this code or make it more obvious.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#to_h
returns a hash ofint => string
. A file can never have a zero line. I thought the existence of it would mean there's an error.First
unless
is when a file is empty. Secondunless
is when there's an error that we need to log.Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense, but why not to throw an exception like any other Ruby code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be unfortunate to miss a notice just because we couldn't read a line of code. I think simply ignoring that error and moving on would be the best solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't suggest you throw that exception to users. You can catch and log it as usually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍